feat(E2E): Enable e2e tests to run in different networks based on chain spec #2403
Conversation
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: Nidhi Singh <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2403 +/- ##
==========================================
- Coverage 32.27% 32.04% -0.24%
==========================================
Files 350 351 +1
Lines 15636 15752 +116
Branches 20 20
==========================================
Hits 5047 5047
- Misses 10226 10342 +116
Partials 363 363
|
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
Signed-off-by: nidhi-singh02 <[email protected]>
nidhi-singh02
changed the title
nidhi-singh02
had a problem deploying
to
test-e2e
GitHub Actions
Failure
nidhi-singh02
temporarily deployed
to
test-e2e
GitHub Actions
Inactive
nidhi-singh02
temporarily deployed
to
test-e2e
GitHub Actions
Inactive
| Note: The default chainID for this local network is 80087, which is our dev network configuration. To make changes to the 80087 chain spec used, modify parameters [here](https://github.com/berachain/beacon-kit/blob/main/config/spec/devnet.go#L40). | ||
|
|
||
| ## Configure the default network configuration | ||
| To change the chainID, modify the `ChainID` field in the `NetworkConfiguration` struct in `defaultNetworkConfiguration` |
There was a problem hiding this comment.
Given that we default to mainnet chain spec in beacond - feels unintuitive to have the default for tests be devnet. Perhaps worth renaming
| //nolint:gochecknoglobals // it's a default value | ||
| var ( | ||
| // defaultChainID is the default chain ID for the network. | ||
| defaultChainID = 80087 |
There was a problem hiding this comment.
Should these be const given that they are not mutated?
| @@ -57,7 +57,11 @@ type ValidatorTestStruct struct { | |||
| // TODO: | |||
| // 1) Add staking tests for adding a new validator to the network. | |||
| // 2) Add staking tests for hitting the validator set cap and eviction. | |||
| func (s *BeaconKitE2ESuite) TestDepositRobustness() { | |||
| func (s *BeaconKitE2ESuite) runDepositRobustness() { | |||
There was a problem hiding this comment.
seems unintuitive to have tests not be named as such - affects readability
There was a problem hiding this comment.
Also my IDE doesn't let me run this test individually now
| networks map[string]*NetworkInstance // maps chainSpec to network | ||
| testSpecs map[string]ChainSpec // maps testName to chainSpec | ||
| testFuncs map[string]func() // maps test names to test functions | ||
| mu sync.RWMutex |
There was a problem hiding this comment.
Can you expand on the reasoning for the mutex? it seems the write lock is only in RegisterTest which doesn't seem to be called concurrently
| To add your tests, you need to do the following: | ||
| 1. Create a new file in the `testing/e2e/` directory. | ||
| 2. Add your tests in here like how it is done in `runBasicStartup()` | ||
| 3. Register your tests in `TestRunE2E()`. |
There was a problem hiding this comment.
Is there any way we can avoid this registration?
There was a problem hiding this comment.
An approach to avoid explicit registration outside of the test is to have each test be "aware" of the chainspec it supports, so that I can look at a test and immediately know what chain spec it's supposed to run on. It would also allow me to run the test directly, without running the whole suite from my IDE (Goland). A problem with "registration" based approaches is also forgetting to register.
Example
func (s *BeaconKitE2ESuite) TestDepositRobustness() {
s.Logger().Info("Running Deposit Robustness on devnet")
network, chainSpec := s.SetForNetwork("devnet")SetForNetwork would effectively run t.Skip() if the test harness is attempting to run the test on a chain spec it doesn't support.
I was initially confused about the modularity goals we aimed to achieve but to be explicit, the user story is as follows:
- I want to be able to run tests that are opinionated for a particular chainspec.
We are NOT trying to do the following:
- Run the same test against multiple chainspecs.
|
+1 to Rez's comment. Some things we should try fixing:
|
|
Will address the review comments/resume here once the node api endpoints for validators is complete, which is priority rn. |
As part of the change set, we will be able to spin up different networks on the basis of chain ID and the chain spec name combination each in its own kurtosis enclave and run the tests.
The instructions to add your tests is given in README.md.
This is the just the foundation of making the e2e tests customizable and being able to run different specs as part of our e2e suite.
Note : Currently only devnet is supported. I see hardcoding of chainspec in few tests, needs to be refactored.